Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add MaskRCNN model as a test for the wrapper approach with TF model garden code #754

Closed
wants to merge 15 commits into from

Conversation

qlzh727
Copy link
Member

@qlzh727 qlzh727 commented Sep 1, 2022

Note that this PR is more for a demo and not intend to be submitted at the moment.

What does this PR do?

Add MaskRCNN model as a test for the wrapper approach with TF model garden code.

  1. The current class only contains the model building logic.
  2. Most of the tunable params are currently backed in the function, and not exposed to use yet.
  3. Verify the model building logic in the test. (will need tfm pip deps to work)

Next step:

  1. Configure the input parsing logic for coco, and also anchor generation.
  2. Mimic the training loop logic as model.train_step/eval_step
  3. Train model e2e.

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue? Please add a link
    to it if that's the case.
  • Did you write any new necessary tests?
  • If this adds a new model, can you run a few training steps on TPU in Colab to ensure that no XLA incompatible OP are used?

Who can review?

@qlzh727
Copy link
Member Author

qlzh727 commented Sep 1, 2022

Comments from the #753:

  1. We should raise model specific error message when TFM is not installed.
  2. The model class interface should be adjusted to follow the Keras CV format.
  3. The function approach should be changed to the model class approach, which has a delegate TFM MaskRCNN in the class body.

@innat
Copy link
Contributor

innat commented Sep 5, 2022

Relevant ticket. #623

Copy link

@martin-gorner martin-gorner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this first attempt.
To bring this further, could you be able to test it actually training, with compile and fit working ?

I have created a starter MaskRCNN notebook here with a small bur real dataset that can be used for further testing:
https://source.cloud.google.com/cloudml-demo-martin/martin-kerascv-test/+/master:Mask_RCNN_test.ipynb
I have not been able to test very far though.

classes=91, # default value for coco
input_size=[512, 512, 3],
backbone="resnet",
backbone_weights=None,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume "resnet" here refers to Model Gaden's ResNet ?
I tried passing "spinenet_mobile" but got an error.

Copy link
Contributor

@bhack bhack Sep 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently supported_premade_backbone = ['resnet', 'spinenet', 'mobilenet']

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried "spinenet" but it errors out:

decoder = tfm.vision.decoders.FPN(
--> 129 input_specs=backbone.output_specs, # Might need to check for custom backbones
130 min_level=min_level,
131 max_level=max_level,

AttributeError: 'str' object has no attribute 'output_specs'

Copy link
Contributor

@bhack bhack Sep 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

)
images = tf.random.uniform(shape=(2, 512, 512, 3))
image_shape = tf.constant([[512, 512], [512, 512]])
anchor_generator = tfm.vision.anchor.build_anchor_generator(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To align with keras_cv, can a default anchor generator be provided when the user instantiates the model?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, for dome reason, tfm.vision.anchor does not work for me. Maybe the symbol is not exported ?

[[200, 200, 300, 300], [300, 300, 400, 400]]])
gt_classes = tf.constant([[1, 2], [3, 4]])
gt_masks = tf.constant(1, shape=[2, 2, 512, 512])
output = mask_rcnn_model(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a single call. Does .fit() work?

def test_maskrcnn_construction(self):
mask_rcnn_model = keras_cv.models.mask_rcnn(
classes=91, # default value for coco
input_size=[512, 512, 3],

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the input_size parameter necessary ?
Usually in Keras models, this gets built from the data.

@martin-gorner
Copy link

martin-gorner commented Sep 6, 2022

Open questions:

  • how should loss configuration work ? There are three losses in MaskRCNN: classification loss, box regression loss and mask loss. An additional complexity id that masks are predicted per class but only the mask loss of the predicted class is taken into account.
  • metric configuration: this should be compatible with KerasCV's COCO metrics.
  • support for KerasCV-style bounding box formats.
  • TPU support ?
  • can data encoding/decoding use KerasCV APIs?

Note: the goal here is to wrap the Model Graden model but not the Model Garden data pipeline code. Users can use Keras preprocessing layers to format a data feed appropriately and we will provide example code in the accompanying templates.

@tanzhenyu
Copy link
Contributor

Should we close this as we most likely will add MaskRCNN on top of existing FasterRCNN?

@LukeWood
Copy link
Contributor

Should we close this as we most likely will add MaskRCNN on top of existing FasterRCNN?

yeah I think so.

@tanzhenyu
Copy link
Contributor

Discussed offline, it seems we all agree to close it for now.

@tanzhenyu tanzhenyu closed this Nov 11, 2022
@qlzh727 qlzh727 deleted the maskrcnn branch September 5, 2023 23:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants